-
-
Notifications
You must be signed in to change notification settings - Fork 522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(plugin-vite): hot-restart 🔥 #3203
Conversation
e6f7c9a
to
e243a4b
Compare
I tried this PR yesterday, and it looks great. |
At present, hot-restart just a simple-helper for dev. process.emit('rs') // restart TODO:
|
I am very bad at understanding where PRs stand ;) is this finished and will be in the next release? |
This feature changes the API design of // vite.main.config.mjs - For Electron Main
// vite.preload.config.mjs - For Preload Scripts
import { defineConfig } from 'vite';
import { restart } from 'electron-forge-plugin-vite/plugin';
// https://vitejs.dev/config
export default defineConfig({
plugins: [restart()],
}); |
@GitMurf We cannot guarantee that. Currently, it appears that @caoxiemeihao still has some features that have not been implemented, so we need to wait until they are completed before conducting a code review. |
Maybe the feature implement here is better 🤔 |
any update on this? |
This PR will reopen when #3178 is merged :) |
e243a4b
to
bd0d802
Compare
bd0d802
to
c09aa2a
Compare
}); | ||
|
||
await viteDevServer.listen(); | ||
viteDevServer.printUrls(); | ||
// viteDevServer.printUrls(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is this commented out intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! When I run test on my local machine, some verbose log print, so commented it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this before we ship it to users then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logs removed here are only for the CI test environment and will not affect the production environment.
Do we need to keep this log in the test environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep this log in the test environment?
No. If others need it, they can add it themselves.
* renderer.find(({ config }) => config.name === 'main_window').reload(); | ||
* ``` | ||
*/ | ||
renderer: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renderer: { | |
renderers: { |
Should this be renderers
since it's a list of all renderer processes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this API design? 🤔
It looks little simper.
export interface VitePluginBuildConfig {
restart?: (rs: (options?: { reload: 'main_window' }) => void) => void;
}
Usage
// Restart App
restart(rs) {
rs();
};
// Reload Renderer process
restart(rs) {
rs({ reload: 'main_window' });
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @erickzhao means that because this is an array, it would be better to use plural for words.
) { | ||
const restart = () => { | ||
// https://github.com/electron/forge/blob/v6.1.1/packages/api/core/src/api/start.ts#L204-L211 | ||
process.stdin.emit('data', 'rs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound pointed out to me that using stdin
here is a bit of a hack here and there are edge cases where process.stdin
could be null.
Ideally, we should instead expose additional start logic from the @electron-forge/core
API to expose this functionality so that plugins can hook into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! This does seem a little dangerous. 🚨
}); | ||
|
||
await viteDevServer.listen(); | ||
viteDevServer.printUrls(); | ||
// viteDevServer.printUrls(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this before we ship it to users then?
Implemented in #3468 |
Summarize your changes:
TODO:
write related test2023-08-09
Hot restart
Main process, this means restart the entire Electron App. ✅Hot reload
Preload scripts, refresh Renderer process can be reload the corresponding Preload script, it's will become better DX. ✅HMR
Renderer process, power by Vite. ✅Custom
restart
orreload
by forge.config.js